Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Promises! #8

Merged
merged 5 commits into from
Apr 21, 2018
Merged

Promises! #8

merged 5 commits into from
Apr 21, 2018

Conversation

steveruizok
Copy link
Contributor

@steveruizok steveruizok commented Apr 20, 2018 •
edited
Loading

I tweaked this project to work on Promises. The changes are non-breaking: the module will still work as usual with callbacks, however each method now returns a Promise as well.

Ex. 1.1
Callback

firebase.get "/users", (users) -> print users

Ex. 1.2
Promise

firebase.get "/users"
.then (users) -> print users

Ex. 1.3
Both also work together

firebase.get "/users", (users) -> print users
.then (users) -> print users

Promises have many advantages, such as handling multiple asynchronous requests using Promise.all.

Ex. 2.1
Multiple requests with callbacks

users = ["user1", "user2", "user3"]
results = []
complete = -> print results

for user in users
    firebase.get "/users/" + user, (data) ->
        results.push(data)
        if results.length is users.length
            complete()

Ex. 2.2
Multiple requests with Promise

promises = ["user1", "user2", "user3"].map (user) -> 
    return firebase.get "/users/" + user

Promise.all(promises)
.then (results) -> print results

This PR also includes a fix for cases where a parameters argument exists where a callback function used to be. In the previous implementation, you wouldn't expect to have a method called with a path and a parameters argument, but without a callback. Offering an alternative to the callback makes a possibility, however, so the module is able to interpret the arguments correctly. Again, the change is non-breaking; both will work correctly.

Ex. 3.1
Parameters but with a callback

printResults = (results) -> print results
firebase.get("/users", printResults, {shallow: true})

Ex. 3.2
Parameters without a callback

firebase.get("/users", {shallow: true})
.then (results) -> print results

Handy!

@marckrenn
Copy link
Owner

Holy Cow, @steveruizok, that's insane! I'm speechless! Thank you so much for this PR, it's amazing 🎉

As you may have noticed during the refactoring of this repo, I wrote it when I had basically no idea what I was doing. And I think it really showed – the code was pretty terrible! But it kinda worked and so I figured, as the lazy person I am, to never touch it again.

And now you went ahead and rewrote it anyway. With promises. And you even updated the readme! That's some true commitment. Wow! I'm really, really grateful. Again, thank you so much.

@marckrenn marckrenn merged commit 2b46f4f into marckrenn:master Apr 21, 2018
@steveruizok
Copy link
Contributor Author

Thanks for the merge! FWIW, I thought the module was really well written and organised. I had to pull out some of the debugging you'd written for the xhttp request, and I haven't really implemented tests/error scenarios, so there are still plenty of areas to improve.

@marckrenn
Copy link
Owner

so there are still plenty of areas to improve.

True. I think I could also

  • make the EventSource-realtime-connection compatible to Framer's autoreloader and
  • reduce the overall code by 25-50% while keeping all the features

If only I had more time the work on fun projects like these …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@steveruizok @marckrenn